cli: hint when install-runner options are ignored#938
cli: hint when install-runner options are ignored#938YasharthPanwar-2003 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to provide users with a hint command when they attempt to install or start a Model Runner that is already active but with different configuration flags. It includes a new helper function to detect flag changes and comprehensive unit tests for the hint generation logic. Feedback was provided regarding a potential security and correctness issue where user-provided flag values are not properly escaped in the suggested command string, which could lead to issues if values contain spaces or malicious characters.
| if backendChanged && opts.backend != "" { | ||
| reinstallArgs = append(reinstallArgs, "--backend", opts.backend) | ||
| } | ||
| if gpuChanged && opts.gpuMode != "" { | ||
| reinstallArgs = append(reinstallArgs, "--gpu", opts.gpuMode) | ||
| } |
There was a problem hiding this comment.
Critical
- The suggested command string is constructed using unvalidated and unescaped user input from flags. This poses a security risk (command injection if a user copy-pastes a malicious string) and a correctness issue if values contain spaces. Values should be properly quoted/escaped for the shell.
Following the Security principle (Repository Style Guide, line 12), input must be handled safely when crossing boundaries, including when displayed back to the user as a suggested command.
| if backendChanged && opts.backend != "" { | |
| reinstallArgs = append(reinstallArgs, "--backend", opts.backend) | |
| } | |
| if gpuChanged && opts.gpuMode != "" { | |
| reinstallArgs = append(reinstallArgs, "--gpu", opts.gpuMode) | |
| } | |
| if backendChanged && opts.backend != "" { | |
| reinstallArgs = append(reinstallArgs, "--backend", fmt.Sprintf("%q", opts.backend)) | |
| } | |
| if gpuChanged && opts.gpuMode != "" { | |
| reinstallArgs = append(reinstallArgs, "--gpu", fmt.Sprintf("%q", opts.gpuMode)) | |
| } |
| @@ -0,0 +1,126 @@ | |||
| package commands | |||
There was a problem hiding this comment.
Better if you can put the tests in existing install-runner_test.go file
There was a problem hiding this comment.
Ok , I do that. May you please help me with if the gemini suggested changes to be done in code ?
There was a problem hiding this comment.
@YasharthPanwar-2003 if you check the code both of theopts.backend and the opts.gpuMode are validated by predefined sepcs. so it won't be a problem though but for defense in future its better to do the suggestion by gemini here like change to the below
if backendChanged && opts.backend != "" {
reinstallArgs = append(reinstallArgs, "--backend", fmt.Sprintf("%q", opts.backend))
}
if gpuChanged && opts.gpuMode != "" {
reinstallArgs = append(reinstallArgs, "--gpu", fmt.Sprintf("%q", opts.gpuMode))
}
Its always important to double check what these analyzers suggest because it sometimes can be wrong. Always try to check whether its the case
There was a problem hiding this comment.
and test this feature in local as well :)
There was a problem hiding this comment.
I dont have NVIDIA gpu cuda , but I tested it over wsl2 Ubuntu 22.04 for cpu only , after the new updation I will test again!
|
@YasharthPanwar-2003 thanks for looking at this, is the bot comment worth fixing? |
|
You will need to fix CI also |
01003e8 to
6b57737
Compare
|
Updated the PR based on review feedback. Summary:
Manual verification:
Validation:
I do not have NVIDIA CUDA hardware, so I did not run the actual vLLM CUDA reinstall path. The manual test verifies the intended existing-runner hint behavior without requiring CUDA. |
| - | ||
| name: Generate matrix | ||
| id: generate | ||
| uses: docker/bake-action/subaction/matrix@a66e1c87e2eca0503c343edf1d208c716d54b8a8 |
There was a problem hiding this comment.
any reason why you change this ?
There was a problem hiding this comment.
Yes. The reason was the failing Validate model-cli / prepare check.
So , previously before new commit , it showed as gvien below . further , I understood to do changes in CI as per asked by ericcurtin
The failure log showed:
actions/github-script@v8 is not allowed in docker/model-runner because all actions must be pinned to a full-length commit SHA
That check comes from this workflow (Validate model-cli) and specifically the prepare job. The direct docker/bake-action/subaction/matrix reference is pinned, but the subaction path was still causing GitHub to resolve actions/github-script@v8, which is blocked by the repo/org full-SHA action policy.
GitHub documents that repositories/organizations can require actions to be pinned to a full-length commit SHA:
https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository
Since cmd/cli/docker-bake.hcl currently has only two validation targets under the validate group (validate-docs and validate-tests), I replaced the dynamic matrix generation with an explicit matrix output while keeping the existing prepare and validate job structure.
If you prefer to keep the bake matrix subaction and handle this CI policy another way, I can revert the workflow change and keep this PR scoped to the install-runner behavior.
There was a problem hiding this comment.
I think we should not do this change in the CI. I think what @ericcurtin mentioned was to fix reason if any changes done from this PR breaks the CI.
There was a problem hiding this comment.
I was thinking same ealier . Thanks for feedback , I will undo it and keep the relevant ones .
Updated based on the review feedback.
I reverted the CI workflow change and kept this PR scoped to the install-runner behavior only.
6b57737 to
a2b06c5
Compare
Summary
install-runnerfinds an existing running Model Runner container--backendor--gpuFixes #907
What changed
commandFlagChangedto check whether a Cobra flag was explicitly changedexistingRunnerOptionsHintto build the reinstall command hintctrID != ""branch to print the hint after the current already-running messagecmd/cli/commands/install_runner_existing_options_hint_test.goWhat this does not change
reinstall-runnerbehaviorTests added
--backendand--gpuwere not explicitly passed--gpu nonehintValidation
go test ./cmd/cli/commands -run "TestInstallRunner|TestExistingRunnerOptionsHint|TestCommandFlagChanged" -vgo test ./...make validate-allValidation details
go.modtidy check passedgolangci-lint run ./...passed with 0 issuesgo test -race ./...passed.versionsis in sync with Dockerfile ARGs